Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add backticks to diff messages for trait changes #2075

Merged
merged 4 commits into from
Jan 24, 2024

Conversation

rchache
Copy link
Contributor

@rchache rchache commented Dec 15, 2023

Description of changes:
Adds backticks (or triple backticks) around trait values within diff event messages. This better supports markdown rendering of the messages.

Also changes pretty-printing to use platform-specific line separator through "%n"

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@rchache rchache requested a review from a team as a code owner December 15, 2023 14:43
@rchache
Copy link
Contributor Author

rchache commented Dec 15, 2023

Tests fail since I used System.lineSeparator() instead of "\n", but the testcases use "\n". Should I change my code to use "\n" or change the testcases to check for System.lineSeparator()?

Copy link
Member

@mtdowling mtdowling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the failing tests, we usually fix the test cases to normalize line endings when we run into these issues. We want to use the right line separator when we write output, but assertions need to be normalized (e.g., find and replace "\r\n" with "\n" on the input and output when doing the assertions).

@rchache rchache requested a review from mtdowling January 9, 2024 02:13
@rchache rchache marked this pull request as draft January 17, 2024 00:08
@rchache rchache force-pushed the backtickImprovement branch from 463cffe to 8cf5028 Compare January 17, 2024 00:25
@rchache rchache force-pushed the backtickImprovement branch from 8cf5028 to c20cbf7 Compare January 17, 2024 00:34
@rchache rchache marked this pull request as ready for review January 17, 2024 00:43
@mtdowling mtdowling merged commit d0551d9 into smithy-lang:main Jan 24, 2024
10 checks passed
hpmellema pushed a commit to hpmellema/smithy that referenced this pull request Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants